Ol5 sidebar #144

umbe1987 · 2018-09-26T21:44:29Z

This PR makes the sidebar compatible with version 5 of OpenLayers, addressing #143.

I don't know if it fits your desire/requirements, but I added:

js/ol-sidebar.js: rewrite of the sidebar code as ES6 module
examples/ol5.js: map creation and addition of the sidebar (don't know if you prefer to keep this in the HTML as the other examples
examples/ol5.html: the HTML part. Here I included a full build of OL5, however one could also use specific imports.

I didn't include the dist/ol5-sidebar.js folder which the HTML <script> is pointing to. I created it using parcel via npm run build.

However, if you consider this PR useful and you'll need that file as well, I'll be happy to provide it.

I am open to any improvements/rework if you think it is a valuable addition to your project.

Thanks!

Turbo87 · 2018-09-27T09:40:01Z

if possible we should avoid to use ES6 directly here. the files in this project should be written in a way that they don't need a build step as the code logic is easy enough and usually doesn't need the complexity of a build step aside from minifying.

umbe1987 · 2018-09-27T10:16:11Z

@Turbo87 ok, I'll try to see if I could make it without the need of a build step then.
Do you perhaps have any suggestions?
I think the first thing would be to remove imports and exports first.
One thing I could do if this is not feasible is to build on my own and just give you the built ol5-sidebar.js if that's fine. Anyway, I'll try to rework on it and come back with my findings.
Thank you for your feedback.

umbe1987 · 2018-09-27T13:07:37Z

Ok I am start thinking it's not a matter of compatibility with OL version 5.

Indeed, from a quick test, if you change the link and script tags in the ol3.html to point (e.g.) the actual version of OL, the code seems to work:

<link rel="stylesheet" href="https://openlayers.org/en/v5.2.0/css/ol.css" type="text/css">
<script src="//openlayers.org/en/v5.2.0/build/ol.js" type="text/javascript"></script>

I think the major problem is that one has to include the full build of OL library (namely, //openlayers.org/en/v5.2.0/build/ol.js), instead of taking advantage of the modern JS modules.
The OpernLayers already moved in this direction (that's what version 5 is all about).

As for the limitation of the actual state of this project to this regard, at present one is forced to adapt the various examples of OpenLayers v5.x to the old style.

Example (taken from Simple Map):

new style (v5.x)

import Map from 'ol/Map.js';
...
var map = new Map({

old style (v4/3.x) (same example)

<script src="https://openlayers.org/en/v4.6.5/build/ol.js"></script>
...
var map = new ol.Map({

This is not a problem per se, nor it breaks functionality. But as it stands, one could not take advantage of the modularity introduced by ES6, amd has to link to the whole ol build.

SOLUTION/PROPOSAL

To be consistent with the architecture of this project, take advantage of the enhancement of this PR, and assure compatibility with the already existing examples, I think it would be possible to include the new ol5-sidebar.js of the js folder AND adding the dist (or whatever you'd like to call it) folder containing the already built library (something similar to Walkermatt/ol-layerswitcher).

Eventually, I think it is enough to:

Update version of OL in the script and link tags (as mentioned before) in the already existing ol3.html (maybe changing the filename to a more generic ol.html?), and
Change <script src="../js/ol3-sidebar.js"></script> to <script src="../dist/ol5-sidebar.js"></script> in the same file.

(Number 1 should work also without merging this PR actually)

I am happy to rework on this PR if you think you'll merge it to your project, and If you don't want to I'd understand it.

umbe1987 · 2018-09-27T16:24:07Z

Just added the dist folder to give the possibility to try the implementation.

umbe1987 · 2018-09-27T17:14:43Z

I think the very last two commits should give more consistency to this PR in your project. Hope to have clarified my proposal and that you might take some time to consider it. Thank you.

umbe1987 · 2018-09-28T22:06:04Z

I just fixed a bug I noticed when the sidebar was closing. All should now be fine and I already tested the use of the fork both in the traditional way and with the new import way and both seem to work fine.

umbe1987 · 2018-10-10T14:22:39Z

@Turbo87 Did you have a chance to consider this PR? If there's anything not clear I'll be happy to clarify, or to make changes as you need.

Turbo87 · 2018-10-10T14:25:59Z

sorry, been on vacation the past few days and driving to a conference right now. I'll try to have a look until next week.

Turbo87

@umbe1987 first of all thanks for the PR! 🙏

the problem here though is that the PR mixes introducing a new build system, with shipping ES6 code, with implementing support for OpenLayers 5. if you think that we keep ES6 code in this project and use a compile step then please work on that first, before introducing new features. with the amount of things changing in this PR I don't feel comfortable merging it right now.

Turbo87 · 2018-10-19T13:44:12Z

package.json

@@ -36,5 +40,8 @@
    "gulp-uglify": "~3.0.0",
    "gulp-zip": "~4.0.0",
    "jshint": "^2.9.5"
+  },
+  "dependencies": {
+    "ol": "^5.2.0"


this will add a dependency to all users, not just those that want the ol5 sidebar

You're totally right. Either I can remove it or move it in the devDependencies. I think the former would be better as sidebar focuses on other mapping libraries.

Turbo87 · 2018-10-19T13:44:21Z

package.json

  },
  "devDependencies": {
+    "cssnano": "^4.1.3",


what is this? why is this needed?

Sincerely I don't know how it got inside! Removing it makes no difference. So, out!

Turbo87 · 2018-10-19T13:45:04Z

package.json

-    "test": "gulp lint"
+    "test": "gulp lint",
+    "start": "parcel ./examples/ol5.html",
+    "build": "parcel build --public-url ./ ./js/ol5-sidebar.js"


we already use gulp for the basic build stuff. why do we need parcel?

Well, I used it because I knew how to use it and not gulp...

Hopefully, there seems to be this guide that says that it is possible "to compile ES6 and beyond into browser-compliant ES5". It seems to be using gulp-babel.

If you want I can dig into this and see what it can do to this regard.

Turbo87 · 2018-10-19T13:45:20Z

package.json

@@ -21,10 +21,14 @@
    "type": "git",
    "url": "https://github.com/turbo87/sidebar-v2.git"
  },
+  "main": "./examples/ol5.js",


this is wrong for all other users that don't use the ol5 asset

That is definitely something I must remove! Sorry.

Turbo87 · 2018-10-19T13:46:04Z

js/ol5-sidebar.js

@@ -0,0 +1,141 @@
+// JS imports
+import Control from 'ol/control/Control';


as I mentioned before this is invalid ES5 code and should not be shipped in the current state

I know, but the example would not use this js but the one built (shipped in dist/ol5-sidebar.js). This is ES5 transpiled code, built with parcel (see the next comment where I say it can be done with gulp as well, although I still need to test it).

This should be there otherwise no one could be able to use the sidebar without using the entire ol library, (which version 5 of OL is stressing not to do), and would force OL5 compliant app to be rewritten from scratch to include the entire ol package (e.g. new Sidebar instead of new ol.control.Sidebar). And this is basically why i made this PR.

umbe1987 · 2018-10-21T19:40:11Z

@Turbo87 Thanks for taking the time to review this PR.

I replied to all your comments and I am writing here a summary.

I know the PR comes with lot of stuff, but I could make it clear saying that the only thing i really wanted to implement was to rewrite the "ol" js as a ES6 module, to guarantee OL5 compatibility.

As for the build process, I explained I used parcel because I simply didn't know how to transpile and build with gulp. Of course, this is something I had to consider, but I neglected it because I mainly focus on providing OL5 compatibility.

I guess I could spend some time to try implementing the build with gulp (mayb trying gulp-babel), removing the necessity of parcel and make another PR. I will keep the ES6 sidebar code as is because it is working and it was my main code to provide.

What do you think? If you are fine with it, feel free to reject this PR and I'll move on with providing another without parcel.

One last thing: I am not an expert programmer so any suggestion from your side (especially regarding gulp) is very welcome! 😸

Turbo87 · 2018-12-07T15:55:24Z

What do you think? If you are fine with it, feel free to reject this PR and I'll move on with providing another without parcel.

as I said above, my goal for now is to keep this repo relatively simple and only include ES5 code. I'm okay with moving towards a solution where we offer ES5 and ESlatest and CJS files, but that shouldn't happen in a PR that implements support for ol5, it should be a dedicated PR.

OpenLayers 5 is implemented as a set of ES modules, requiring major changes to the sidebar. Depending on how OL5 is used, the sidebar will either be used as an ES module as well, or, in case the full build of OL (ol.js) is used, a version compiled via webpack for direct usage in a browser environment is required.

Reworked integration of OL5 support.

severinstrobl · 2019-04-30T11:23:55Z

The updated pull request contains some modifications trying to integrate the OL5 support better with the other versions of the sidebar.
@Turbo87 Due to the fundamentally different nature of OL5 based on ES modules the sidebar code also has to be handled differently, but this is closer to what I feel you have in mind. Do you think this is acceptable for merging?

umbe1987 · 2019-10-10T10:14:11Z

@Turbo87
As this is PR getting pretty old (OpenLayers has now moved to version 6), would the improvements introduced by @severinstrobl be good enough to make this PR finally acceptable or do you think it would be better to close and rethinking it?

Turbo87 · 2019-11-21T18:57:12Z

I'm still not sure why we need all of these changes. It looks like #156 achieved ol5/6 compatibility with much less changes 🤔

severinstrobl · 2019-11-22T11:41:51Z

The changes in #156 are indeed minimal and are sufficient to use OpenLayers in the traditional way via the full JS build, while the modern and recommended way relies on ES modules. So IMHO in order for the sidebar to also be usable in more complex applications, the changes introduced in this PR are mandatory.

umbe1987 · 2019-11-22T12:58:02Z

To add to what @severinstrobl correctly said, since the advent of OpenLayers v5, many modern application based on OL has adopted the modern ES module import to avoid importing the whole OL library, hence decreasing the size of the final application in production.

Before this introduction, the size of the OL library was a major CON with respect to other alternatives (e.g. leaflet).

As it stands, the sidebar forces to include the entire OL build. This PR would make it possible to use the Sidebar with only the OL modules that it requires instead, still preserving the compatibility for the other libraries as it is now.

umbe1987 added 7 commits September 24, 2018 22:40

First (failed) round of ol5-ying the sidebar

d65fb01

first (nasty) working example

30f850f

major reworking. Need to fix i, child lacking in defaults

22f209b

Slight adjustments to method calls and CSS import in HTML

ca3b0c9

super.setMap(map);

61e5a8e

defaults added to options

0553917

fixed open call and final working result

08e6403

Turbo87 added the enhancement label Sep 27, 2018

added dist folder with bundled js

12527c0

umbe1987 added 2 commits September 27, 2018 19:03

updated link and script refs to latest ol version

137e2c9

incorporated js logic in ol5.html for project consistency

bb185d3

fixed sidebar.close

981287a

Turbo87 requested changes Oct 19, 2018

View reviewed changes

severinstrobl and others added 2 commits April 30, 2019 11:16

Merge pull request #1 from severinstrobl/ol5-sidebar

a4acfa9

Reworked integration of OL5 support.

umbe1987 requested a review from Turbo87 November 19, 2019 20:21

paul121 mentioned this pull request Sep 21, 2020

Put layer switcher into sidebar-v2 farmOS/farmOS-map#14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ol5 sidebar #144

Ol5 sidebar #144

umbe1987 commented Sep 26, 2018

Turbo87 commented Sep 27, 2018

umbe1987 commented Sep 27, 2018

umbe1987 commented Sep 27, 2018 •

edited

Loading

umbe1987 commented Sep 27, 2018

umbe1987 commented Sep 27, 2018

umbe1987 commented Sep 28, 2018

umbe1987 commented Oct 10, 2018

Turbo87 commented Oct 10, 2018

Turbo87 left a comment

Turbo87 Oct 19, 2018

umbe1987 Oct 21, 2018

Turbo87 Oct 19, 2018

umbe1987 Oct 21, 2018

Turbo87 Oct 19, 2018

umbe1987 Oct 21, 2018

Turbo87 Oct 19, 2018

umbe1987 Oct 21, 2018

Turbo87 Oct 19, 2018

umbe1987 Oct 21, 2018 •

edited

Loading

umbe1987 commented Oct 21, 2018 •

edited

Loading

Turbo87 commented Dec 7, 2018

severinstrobl commented Apr 30, 2019

umbe1987 commented Oct 10, 2019

Turbo87 commented Nov 21, 2019

severinstrobl commented Nov 22, 2019

umbe1987 commented Nov 22, 2019

		@@ -0,0 +1,141 @@
		// JS imports
		import Control from 'ol/control/Control';

Ol5 sidebar #144

Are you sure you want to change the base?

Ol5 sidebar #144

Conversation

umbe1987 commented Sep 26, 2018

Turbo87 commented Sep 27, 2018

umbe1987 commented Sep 27, 2018

umbe1987 commented Sep 27, 2018 • edited Loading

umbe1987 commented Sep 27, 2018

umbe1987 commented Sep 27, 2018

umbe1987 commented Sep 28, 2018

umbe1987 commented Oct 10, 2018

Turbo87 commented Oct 10, 2018

Turbo87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umbe1987 Oct 21, 2018 • edited Loading

Choose a reason for hiding this comment

umbe1987 commented Oct 21, 2018 • edited Loading

Turbo87 commented Dec 7, 2018

severinstrobl commented Apr 30, 2019

umbe1987 commented Oct 10, 2019

Turbo87 commented Nov 21, 2019

severinstrobl commented Nov 22, 2019

umbe1987 commented Nov 22, 2019

umbe1987 commented Sep 27, 2018 •

edited

Loading

umbe1987 Oct 21, 2018 •

edited

Loading

umbe1987 commented Oct 21, 2018 •

edited

Loading